Check the network access when deploying VM in Advanced Security Group. by sureshanaparti · Pull Request #6050 · apache/cloudstack

sureshanaparti · 2022-03-01T07:11:27Z

Description

This PR checks the network access when deploying VM in Advanced Security Group.

Fixes #6049

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Before fix, able to deploy VM with ROOT account, in Domain02 using network on Domain01 (in Advanced Security Group)
After fix, deploy VM failed with 'Unable to use network with id= 031a45f3-8522-4265-a145-6a647df20d27, permission denied'

sureshanaparti · 2022-03-01T07:12:19Z

@blueorangutan package

blueorangutan · 2022-03-01T07:13:03Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-03-01T07:47:50Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2734

sureshanaparti · 2022-03-01T10:07:26Z

@blueorangutan package

blueorangutan · 2022-03-01T10:08:03Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-03-01T10:42:57Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2735

nvazquez · 2022-03-01T14:10:31Z

@GabrielBrascher can you please review/test this fix?

sureshanaparti · 2022-03-01T14:32:59Z

@blueorangutan test

blueorangutan · 2022-03-01T14:34:02Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

weizhouapache

code lgtm !!

weizhouapache · 2022-03-01T15:52:21Z

@sureshanaparti
can you also add my suggestion in comment #6049 (comment) to this PR ?

blueorangutan · 2022-03-02T02:00:11Z

Trillian test result (tid-3451)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39790 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6050-t3451-kvm-centos7.zip
Smoke tests completed. 83 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

sureshanaparti · 2022-03-02T08:22:28Z

@sureshanaparti
can you also add my suggestion in comment #6049 (comment) to this PR ?

@weizhouapache the access check for VM owner already exists here.

cloudstack/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

Lines 1312 to 1313 in 85c5997

    
           Account vmOwner = _accountMgr.getAccount(vmInstance.getAccountId()); 
        
           _networkModel.checkNetworkPermissions(vmOwner, network);

so, the check against caller is still required here?

cloudstack/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

Lines 1339 to 1340 in 85c5997

    
           // Perform account permission check on network 
        
           _accountMgr.checkAccess(caller, AccessType.UseEntry, false, network);

weizhouapache · 2022-03-02T09:46:57Z

so, the check against caller is still required here?

@sureshanaparti
then the 2nd check can be removed

sureshanaparti · 2022-03-02T10:52:04Z

@blueorangutan package

GabrielBrascher · 2022-03-02T17:29:19Z

Thanks for the PR @sureshanaparti!
Is this ready to review or still in Draft?

I did a few tests applying your changes and seems to be working fine.
I will be running a few more tests but wanted to double-check if there is anything left for the PR.

sureshanaparti · 2022-03-03T04:50:39Z

Thanks for the PR @sureshanaparti! Is this ready to review or still in Draft?

I did a few tests applying your changes and seems to be working fine. I will be running a few more tests but wanted to double-check if there is anything left for the PR.

it is ready for review @GabrielBrascher

sureshanaparti · 2022-03-03T04:51:22Z

@blueorangutan package

blueorangutan · 2022-03-03T04:52:03Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-03-03T05:26:59Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2753

weizhouapache · 2022-03-03T07:13:06Z

@sureshanaparti
should this target to 4.16 ?

GabrielBrascher

@thanks @sureshanaparti, code LGTM.
Tested With Advanced Network + SG and now it is working as expected.

sureshanaparti · 2022-03-03T08:49:42Z

@thanks @sureshanaparti, code LGTM.
Tested With Advanced Network + SG and now it is working as expected.

great, thanks for testing @GabrielBrascher

sureshanaparti · 2022-03-03T08:51:04Z

@sureshanaparti
should this target to 4.16 ?

@weizhouapache this is targeted to 4.17

weizhouapache · 2022-03-03T08:54:31Z

if there is 4.16.2.0 release, we need to backport this, right ?
why not target to 4.16 ?

sureshanaparti · 2022-03-03T09:04:34Z

if there is 4.16.2.0 release, we need to backport this, right ?
why not target to 4.16 ?

yes, may be when another minor release is needed on 4.16.

nvazquez · 2022-03-04T16:49:50Z

Agree with @weizhouapache - let's target the fix for branch 4.16 and merge forward to main

…ists

nvazquez · 2022-03-04T17:03:24Z

@blueorangutan package

blueorangutan · 2022-03-04T17:04:03Z

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-03-04T17:40:57Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2766

nvazquez · 2022-03-04T17:59:27Z

@blueorangutan test

nvazquez

LGTM

blueorangutan · 2022-03-04T18:00:03Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2022-03-05T13:46:26Z

Trillian test result (tid-3496)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36085 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6050-t3496-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

sureshanaparti force-pushed the network-access-check-improvements branch from fa1f91b to f338ab1 Compare March 1, 2022 09:58

sureshanaparti linked an issue Mar 1, 2022 that may be closed by this pull request

CloudStack allows deploying VMs on Networks that should not be accessed by VM's owner #6049

Closed

sureshanaparti requested review from GabrielBrascher, nvazquez and weizhouapache March 1, 2022 14:31

sureshanaparti added the component:networking label Mar 1, 2022

sureshanaparti self-assigned this Mar 1, 2022

weizhouapache approved these changes Mar 1, 2022

View reviewed changes

sureshanaparti added this to the 4.17.0.0 milestone Mar 2, 2022

sureshanaparti force-pushed the network-access-check-improvements branch from cc724ed to d64d936 Compare March 2, 2022 10:49

sureshanaparti marked this pull request as ready for review March 3, 2022 04:50

GabrielBrascher approved these changes Mar 3, 2022

View reviewed changes

apache deleted a comment from blueorangutan Mar 4, 2022

nvazquez force-pushed the network-access-check-improvements branch from d64d936 to fd28dd5 Compare March 4, 2022 16:57

nvazquez changed the base branch from main to 4.16 March 4, 2022 16:58

sureshanaparti added 3 commits March 4, 2022 14:02

Check the network access when deploying VM in Advanced Security Group.

8614ca8

Removed comment

68370cc

Removed redundant network access check, owner access check already ex…

fec436b

…ists

nvazquez force-pushed the network-access-check-improvements branch from fd28dd5 to fec436b Compare March 4, 2022 17:02

nvazquez approved these changes Mar 4, 2022

View reviewed changes

nvazquez merged commit 2820a36 into apache:4.16 Mar 6, 2022

yadvr added the type:bug label Mar 24, 2022

Conversation

sureshanaparti commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

sureshanaparti commented Mar 1, 2022

Uh oh!

blueorangutan commented Mar 1, 2022

Uh oh!

blueorangutan commented Mar 1, 2022

Uh oh!

sureshanaparti commented Mar 1, 2022

Uh oh!

blueorangutan commented Mar 1, 2022

Uh oh!

blueorangutan commented Mar 1, 2022

Uh oh!

nvazquez commented Mar 1, 2022

Uh oh!

sureshanaparti commented Mar 1, 2022

Uh oh!

blueorangutan commented Mar 1, 2022

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Mar 1, 2022

Uh oh!

blueorangutan commented Mar 2, 2022

Uh oh!

sureshanaparti commented Mar 2, 2022

Uh oh!

weizhouapache commented Mar 2, 2022

Uh oh!

sureshanaparti commented Mar 2, 2022

Uh oh!

GabrielBrascher commented Mar 2, 2022

Uh oh!

sureshanaparti commented Mar 3, 2022

Uh oh!

sureshanaparti commented Mar 3, 2022

Uh oh!

blueorangutan commented Mar 3, 2022

Uh oh!

blueorangutan commented Mar 3, 2022

Uh oh!

weizhouapache commented Mar 3, 2022

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Mar 3, 2022

Uh oh!

sureshanaparti commented Mar 3, 2022

Uh oh!

weizhouapache commented Mar 3, 2022

Uh oh!

sureshanaparti commented Mar 3, 2022

Uh oh!

nvazquez commented Mar 4, 2022

Uh oh!

nvazquez commented Mar 4, 2022

Uh oh!

blueorangutan commented Mar 4, 2022

Uh oh!

blueorangutan commented Mar 4, 2022

Uh oh!

nvazquez commented Mar 4, 2022

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Mar 4, 2022

Uh oh!

sureshanaparti commented Mar 1, 2022 •

edited

Loading